fix(ts-sdk): decode base64 data URLs with media-type parameters - #247
fix(ts-sdk): decode base64 data URLs with media-type parameters#247rohitsux wants to merge 3 commits into
Conversation
toImageBytes()'s regex ^data:[^;]+;base64, requires a ;-free media type, so valid RFC 2397 data URLs with a media-type parameter (e.g. data:image/svg+xml;charset=utf-8;base64,...) or an omitted media type (data:;base64,...) don't match and fall through, handing the entire data URL to the base64 decoder. That decoder then throws InvalidCharacterError under atob (browser) or silently corrupts bytes under Buffer.from (Node). Fix matches up to the ;base64, marker ([^,]*) instead. Base64 payloads never contain a comma, so the payload is still captured correctly. No change in behavior for existing inputs. Added two regression tests covering a media type with a parameter and an omitted media type.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughChangesImage data URL decoding
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Image data URLs now accept valid base64 variations and return clear errors for malformed or non-base64 inputs. The covered behavior is ready to merge with no identified current-head risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
mamayer19
left a comment
mamayer19
left a comment
There was a problem hiding this comment.
Thanks for the contribution, this reproduces the original issue clearly, and both new cases pass locally. I left one suggestion about parsing the data URL structurally instead of broadening the regex, along with a few related edge cases that would make the behavior explicit.
| // everything up to the ";base64," marker rather than a single ";"-free | ||
| // segment — otherwise such URLs fall through and the whole data URL is | ||
| // handed to the base64 decoder (corrupting the bytes or throwing). | ||
| const dataUrlMatch = input.match(/^data:[^,]*;base64,(.+)$/); |
There was a problem hiding this comment.
Could we avoid encoding the data-URL grammar in a regex here? Since the format has a well-defined comma delimiter, a small helper could find the first comma and inspect the semicolon-delimited metadata before it for the final base64 marker. That would be easier to follow and give us a natural place to handle the scheme and marker case-insensitively, percent-decode the payload, and return a clear error for malformed data: input.
As written, inputs such as DATA:image/png;BASE64,SGVsbG8%3D still miss the match and are passed to the raw-base64 decoder. Would you be open to extracting something like parseBase64DataUrl(input): string | undefined, leaving base64ToBytes responsible only for decoding the extracted payload?
| it("decodes a data URL whose media type carries a parameter", async () => { | ||
| // Valid per RFC 2397: the media type may be followed by ";param=value" | ||
| // (e.g. charset) before ";base64,". "Hello" base64-encoded. | ||
| const dataUrl = "data:image/svg+xml;charset=utf-8;base64,SGVsbG8="; |
There was a problem hiding this comment.
These two regression cases are a good start. If we switch to a small parser, could we also cover an uppercase scheme/BASE64 marker, percent-escaped padding such as %3D, and one explicit empty-payload case? Those are the boundaries where the current regex falls through into the raw-base64 path, and the tests would make the intended behavior clear.
…ases Replace the data-URL regex in toImageBytes with a parseBase64DataUrl helper that walks the RFC 2397 grammar structurally: it splits on the first comma, matches the data: scheme and the base64 marker case-insensitively, percent-decodes the payload, and throws a clear error for a malformed or non-base64 data: URL instead of silently handing it to the base64 decoder. base64ToBytes is now responsible only for decoding the extracted payload. Add tests for an uppercase scheme and BASE64 marker, percent-escaped padding (%3D), an empty payload, and the two clear-error cases.
|
Thanks for the review — reworked it along your suggestion.
Full SDK suite is green (513) with typecheck + biome clean. Happy to adjust the error wording if you'd prefer something different. |
| // base64 string (which can never begin with "data:", since ":" is not a | ||
| // base64 character) is decoded as-is. | ||
| const payload = parseBase64DataUrl(input); | ||
| return base64ToBytes(payload ?? input); |
There was a problem hiding this comment.
One behavior suggestion: data:;base64, now returns an empty Uint8Array and succeeds. That zero-byte "image" then goes out over the wire as { data: , format: "jpeg" } and fails somewhere in the server, far from the actual mistake. Empty payload should throw too.
3 participants
Problem
toImageBytes()inpackages/sie_ts_sdk/src/images.tsdetects base64 data URLs with the regex:This requires the media type segment to contain no
;. Per RFC 2397, though, the media type may carry parameters (e.g.;charset=utf-8) or be omitted entirely. Data URLs like:data:image/svg+xml;charset=utf-8;base64,...data:;base64,...don't match this pattern and fall through to the plain base64 branch, which hands the entire data URL string (including the
data:...;base64,prefix) to the base64 decoder. That decoder then either throwsInvalidCharacterErrorunderatob(browser) or silently produces corrupted bytes underBuffer.from(Node) — neither of which surfaces as a clear "unsupported input" error.Fix
Match up to the
;base64,marker instead of requiring a;-free segment:Base64 payloads never contain a comma, so the capture group still correctly isolates just the payload. Behavior for all previously-matching data URLs is unchanged.
Tests
Added two regression tests to
packages/sie_ts_sdk/tests/images.test.ts:image/svg+xml;charset=utf-8)data:;base64,...)Verified locally: fails-before (both new tests throw on the unpatched regex) / passes-after.
tests/images.test.ts17/17; full@superlinked/sie-sdksuite 484/484;biome checkclean;tsc --noEmit(typecheck) clean.Summary by CodeRabbit
Bug Fixes
Tests